Slash command menu working.#9358
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
I'm starting a first review of this pull request. You can follow along in the session on Warp. I completed the review and posted feedback on this pull request. Comment I completed the review and posted feedback on this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR adds a Cloud Mode V2 slash-command menu, including a new floating view, zero-state grouping for commands/skills/prompts, compact slash-command item rendering, and routing of keyboard acceptance/navigation to the new view when composing in Cloud Mode V2.
Concerns
- The new click handler in
CloudModeV2SlashCommandView::rebuild_from_resultscaptures and moves an unused weak handle, which makes the callback fail the renderer'sFnrequirement and blocks compilation.
Verdict
Found: 1 critical, 0 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| item, | ||
| cmd_or_ctrl_enter: false, | ||
| }); | ||
| let _ = weak_handle; |
There was a problem hiding this comment.
🚨 [CRITICAL] This let _ = weak_handle; moves the captured WeakViewHandle out of the click closure, so the closure is only FnOnce and cannot satisfy QueryResultRenderer::new's Fn bound or be reused for every renderer. Remove the unused weak-handle capture/field, or create a fresh clone per renderer if the callback needs it.
There was a problem hiding this comment.
Overview
This PR adds a Cloud Mode V2 slash-command menu, new sectioned zero-state results for commands/skills/prompts, compact slash-command item rendering, and routes keyboard acceptance/navigation to the new menu while composing.
Concerns
- The new cloud-mode view currently captures and moves
weak_handleinside its click callback, which makes the callback incompatible with theFncallback expected byQueryResultRenderer::newand reused for each renderer. - The shared slash-command data source is switched to compact/v2 behavior whenever
CloudModeInputV2is enabled, even though the regular inline slash menu still uses the same handle outside cloud-mode composing. - Security pass: no security-specific findings in the changed lines.
Verdict
Found: 1 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| item, | ||
| cmd_or_ctrl_enter: false, | ||
| }); | ||
| let _ = weak_handle; |
There was a problem hiding this comment.
🚨 [CRITICAL] let _ = weak_handle; moves the captured handle out of the click closure, so the closure is FnOnce while QueryResultRenderer::new requires an Fn and this same callback is reused for every renderer; remove this capture or clone per renderer.
| terminal_view_id, | ||
| }; | ||
| if FeatureFlag::CloudModeInputV2.is_enabled() { | ||
| SlashCommandDataSource::for_cloud_mode_v2(args, ctx) |
There was a problem hiding this comment.
slash_command_data_source to compact/v2 rendering for every slash menu whenever the flag is enabled, but the regular inline slash menu still receives this handle outside is_cloud_mode_input_v2_composing; keep the default source for the inline view or create a separate v2 source for CloudModeV2SlashCommandView.
abhishekp106
force-pushed
the
abhishek/slash-command-menu
branch
from
9c0eb09 to
849517c
Compare
abhishekp106
force-pushed
the
abhishek/slash-command-menu
branch
from
21bc4dc to
a9aebec
Compare
liliwilson
left a comment
liliwilson
left a comment
There was a problem hiding this comment.
Sweet!
A few things I am noticing while playing around:
- Inserting a skill into the input + hitting enter doesn't kick off a cloud agent
- Inserting a saved prompt into the input renders what I think is the saved prompt card at the top of the screen
- Some of the slash commands don't work and then prevent you from opening the command menu again (e.g. `/prompts`) - might just be some missing plumbing
| pub terminal_view_id: EntityId, | ||
| } | ||
|
|
||
| pub struct SlashCommandDataSource { |
There was a problem hiding this comment.
(Non-blocking) This is somewhat of a preexisting bug, but could we add something like an allowed_commands filter here that filters out commands that don't make sense in this cloud input view?
Thinking of things like /create-environment, /cloud-agent, /compact, /cost, /export..., /fork, /queue, /remote-control, /rewind
There was a problem hiding this comment.
just did this here because there's a bunch of commands that don't apply and making the correct UI behavior happen is extra work!
We also should eventually not show /fork and a bunch of others at the beginning of a conversation too
There was a problem hiding this comment.
My list is slightly different than yours, but lmk if there's something that you didn't expect!
| } | ||
| } | ||
|
|
||
| fn next_selectable_browsing_idx( |
There was a problem hiding this comment.
nit: can we genericify this fn and the next one?
| #[allow(dead_code)] | ||
| fn _icon_size() -> f32 { | ||
| ICON_SIZE | ||
| } | ||
|
|
||
| #[allow(dead_code)] | ||
| fn _icon_to_text_gap() -> f32 { | ||
| ICON_TO_TEXT_GAP | ||
| } |
2 participants
Description
Adds the new slash command menu for cloud mode v2.
https://www.loom.com/share/12c523ae66ca4e26b2d2ef46fdbcb035
Testing
See loom.